Skip to content

Upgrade pymongo - #34675

Merged
mumarkhan999 merged 1 commit into
masterfrom
umar/upgrade-pymongo
Jul 25, 2024
Merged

Upgrade pymongo#34675
mumarkhan999 merged 1 commit into
masterfrom
umar/upgrade-pymongo

Conversation

@mumarkhan999

@mumarkhan999 mumarkhan999 commented May 2, 2024

Copy link
Copy Markdown
Contributor

Issue Link

Description

Related PRs

@mumarkhan999
mumarkhan999 force-pushed the umar/upgrade-pymongo branch from 343bc83 to bb7d0f2 Compare May 2, 2024 11:45
@mumarkhan999
mumarkhan999 requested a review from a team as a code owner May 2, 2024 11:45
@mumarkhan999
mumarkhan999 force-pushed the umar/upgrade-pymongo branch 11 times, most recently from b0683c5 to 4139dea Compare May 3, 2024 15:46
@mumarkhan999
mumarkhan999 force-pushed the umar/upgrade-pymongo branch 4 times, most recently from 424c152 to 42fd48e Compare May 22, 2024 07:10
@mumarkhan999 mumarkhan999 reopened this May 22, 2024
@mumarkhan999
mumarkhan999 force-pushed the umar/upgrade-pymongo branch 11 times, most recently from 39a0ef6 to dd4fca6 Compare May 22, 2024 13:14
@mumarkhan999
mumarkhan999 force-pushed the umar/upgrade-pymongo branch 4 times, most recently from a2a594d to a0c6c6d Compare June 21, 2024 09:34
@mumarkhan999
mumarkhan999 force-pushed the umar/upgrade-pymongo branch 7 times, most recently from a1305b0 to 849d7c0 Compare July 9, 2024 10:42

@feanil feanil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions but this generally makes sense to me.

@@ -1,3 +0,0 @@
# We want to ignore files in this directory which we do in the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this message? It seems useful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is temporary to make sandbox builds successful.
Will be reverted before this pr is merged.

Comment thread xmodule/contentstore/mongo.py Outdated
thumbnail_location[4]
)

# md5 = getattr(fp, 'md5', None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these comments still needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed. Removed.

@mumarkhan999
mumarkhan999 force-pushed the umar/upgrade-pymongo branch 2 times, most recently from 1a06f6a to b840069 Compare July 18, 2024 13:03

@feanil feanil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small question, but the approach generally seems good, There are a lot of new very small functions check_connection do_connection, etc. They made it harder for me to read through what was happening, was there a reason for making these new functions?

Comment thread requirements/edx/base.txt Outdated
camel-converter[pydantic]==3.1.2
# via meilisearch
celery==5.4.0
celery==5.3.6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is celery being downgraded?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mumarkhan999 still waiting for an answer to why this happened.

@mumarkhan999

mumarkhan999 commented Jul 19, 2024

Copy link
Copy Markdown
Contributor Author

One small question, but the approach generally seems good, There are a lot of new very small functions check_connection do_connection, etc. They made it harder for me to read through what was happening, was there a reason for making these new functions?

These functions are needed to manage the availability of MongoDB connections.

In pymongo versions >4.x.xx the reconnection functionality has been removed.
(More info is available in the changelog) Now, these functions are making sure that our connection is open while making any MongoDB request.

@mumarkhan999
mumarkhan999 requested a review from feanil July 19, 2024 14:09

@feanil feanil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me and makes sense. I just have one last question about the downgrade of the celery dependency.

Comment thread requirements/edx/base.txt Outdated
camel-converter[pydantic]==3.1.2
# via meilisearch
celery==5.4.0
celery==5.3.6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mumarkhan999 still waiting for an answer to why this happened.

@mumarkhan999
mumarkhan999 force-pushed the umar/upgrade-pymongo branch 3 times, most recently from 7e42749 to 9e6af2d Compare July 23, 2024 12:44

@feanil feanil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small nits and suggestion but I think once those are addressed this can be merged. I don't need to do another round of review unless there are significant changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder to revert this change before merging.

Comment thread requirements/constraints.txt Outdated
@@ -34,7 +34,10 @@ django-simple-history==3.4.0

# constrained in opaque_keys. migration guide here: https://pymongo.readthedocs.io/en/4.0/migrate-to-pymongo4.html

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this comment need to be updated now?

pymongo<4.0.0
pymongo<4.4.1

# To override the constraint of edx-lint

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# To override the constraint of edx-lint
# To override the constraint of edx-lint
# This can be removed once https://github.com/openedx/edx-platform/issues/34586 is resolved
# and the upstream constraint in edx-lint has been removed.

This was referenced Jul 25, 2024
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants